feat: add --replay mode for JSONL event replay without eBPF#1010
feat: add --replay mode for JSONL event replay without eBPF#1010Molter73 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds a ChangesReplay mode and task orchestration refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Molter73
force-pushed
the
mauro/feat/replay-input
branch
from
17c817d to
b4c135f
Compare
Add a --replay <file> CLI flag that reads pre-recorded events from a JSONL file instead of loading eBPF programs. This allows profiling the entire userspace pipeline with tools like valgrind DHAT that don't support BPF syscalls. In replay mode, the BPF loader and HostScanner are bypassed entirely. Events flow directly from the JSONL reader through the RateLimiter and Output stages. Changes: - Add Deserialize impls for inode_key_t and monitored_t (fact-ebpf) - Add Deserialize derives to Event, FileData, Process and related types - Make kernel metrics optional in Exporter (None in replay mode) - Migrate task handles to a shared JoinSet for cleaner lifecycle mgmt - Add replay module with async JSONL file reader - Add --replay flag to CLI and config Assisted-by: `agent-mode` <noreply@opencode.ai>
Molter73
force-pushed
the
mauro/feat/replay-input
branch
from
b4c135f to
68ec0c4
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1010 +/- ##
==========================================
- Coverage 32.65% 31.69% -0.97%
==========================================
Files 21 22 +1
Lines 2915 3007 +92
Branches 2915 3007 +92
==========================================
+ Hits 952 953 +1
- Misses 1960 2049 +89
- Partials 3 5 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/event/mod.rs (1)
73-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
hostnamewhen replaying events.#[serde(skip_deserializing)]drops it to""onEventdeserialization, andfact/src/replay.rsforwards that value unchanged intofact_api::FileActivity. Replayed output loses the recorded host identity; add a deserialization default or repopulate it in the replay path if that’s not intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/event/mod.rs` around lines 73 - 80, The Event deserialization currently drops hostname to an empty value, which then gets forwarded unchanged by the replay path. Update the Event type’s serde handling so hostname is preserved or restored during deserialization, and verify the replay logic in replay.rs / FileActivity construction uses the recorded hostname instead of the blank default.
🧹 Nitpick comments (2)
fact-ebpf/src/lib.rs (1)
108-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider round-trip tests for the new Deserialize impls.
No tests exercise serialize→deserialize round trips for
inode_key_t/monitored_t, matching Codecov's coverage gap on this file. A couple of small unit tests would catch drift between the manualSerialize/Deserializeimpls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact-ebpf/src/lib.rs` around lines 108 - 194, Add small unit tests for the new manual serde implementations in inode_key_t and monitored_t to cover serialize-to-deserialize round trips. Use the existing Serialize/Deserialize impls for inode_key_t and monitored_t to verify that a value serialized to JSON (or equivalent serde format) deserializes back to the same value, including at least one case for each monitored_t variant and a representative inode_key_t value. Place the tests near the serde impls in lib.rs so they clearly exercise these symbols and prevent drift between the custom serialization and deserialization logic.fact/src/replay.rs (1)
13-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo tests for the replay reader.
Matches Codecov's coverage gap for this file. A test spawning
start()against a small temp JSONL (including a malformed line) would validate the parse/skip/forward behavior cheaply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/replay.rs` around lines 13 - 61, Add coverage for replay::start by introducing a test that spawns it with a temporary JSONL file and a watch receiver, then verifies the reader forwards valid Event entries, skips malformed JSON lines, and continues after deserialize failures. Use the start function and its line-by-line parsing path in replay.rs so the test exercises the existing parse/skip/forward behavior end to end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fact/src/config/mod.rs`:
- Line 44: The new replay field in the configuration schema is missing dedicated
test coverage. Update fact/src/config/tests.rs to add cases that parse a YAML
replay: <path> value, exercise the replay() accessor, and verify update() merges
a Some(path) value correctly; use the existing Config and update() test patterns
as the reference point. Also extend the fully specified config fixtures that
already include replay: None so they cover the new field consistently, but make
sure there is at least one explicit test for parsing and merging replay.
In `@fact/src/event/process.rs`:
- Around line 41-48: The Process.username field is only marked
skip_deserializing, so replayed events keep the default empty value and never
get repopulated. Update the Process struct’s serde handling to mirror the
Event.hostname fix by adding a default provider (or equivalent reassignment
path) for username in fact/src/event/process.rs, and ensure the
replay/deserialization flow restores a usable username instead of leaving it
blank.
In `@fact/src/replay.rs`:
- Around line 13-61: The replay task in start() is being treated as a global
shutdown signal when it reaches EOF, which can stop downstream consumers before
their bounded queues are drained. Update the replay completion path so finishing
the file does not flip the shared running flag in run(); instead, keep replay
completion separate from shutdown or add an explicit drain/flush step for
RateLimiter and Output before exit. Use the start() task and the
run()/running.changed() shutdown flow to locate the fix.
---
Outside diff comments:
In `@fact/src/event/mod.rs`:
- Around line 73-80: The Event deserialization currently drops hostname to an
empty value, which then gets forwarded unchanged by the replay path. Update the
Event type’s serde handling so hostname is preserved or restored during
deserialization, and verify the replay logic in replay.rs / FileActivity
construction uses the recorded hostname instead of the blank default.
---
Nitpick comments:
In `@fact-ebpf/src/lib.rs`:
- Around line 108-194: Add small unit tests for the new manual serde
implementations in inode_key_t and monitored_t to cover serialize-to-deserialize
round trips. Use the existing Serialize/Deserialize impls for inode_key_t and
monitored_t to verify that a value serialized to JSON (or equivalent serde
format) deserializes back to the same value, including at least one case for
each monitored_t variant and a representative inode_key_t value. Place the tests
near the serde impls in lib.rs so they clearly exercise these symbols and
prevent drift between the custom serialization and deserialization logic.
In `@fact/src/replay.rs`:
- Around line 13-61: Add coverage for replay::start by introducing a test that
spawns it with a temporary JSONL file and a watch receiver, then verifies the
reader forwards valid Event entries, skips malformed JSON lines, and continues
after deserialize failures. Use the start function and its line-by-line parsing
path in replay.rs so the test exercises the existing parse/skip/forward behavior
end to end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 9570c942-4422-4544-b4e3-40dde97bdb37
📒 Files selected for processing (14)
CHANGELOG.mdfact-ebpf/src/lib.rsfact/src/bpf/mod.rsfact/src/config/mod.rsfact/src/config/reloader.rsfact/src/config/tests.rsfact/src/event/mod.rsfact/src/event/process.rsfact/src/host_scanner.rsfact/src/lib.rsfact/src/metrics/exporter.rsfact/src/output/mod.rsfact/src/rate_limiter.rsfact/src/replay.rs
| hotreload: Option<bool>, | ||
| scan_interval: Option<Duration>, | ||
| rate_limit: Option<u64>, | ||
| replay: Option<PathBuf>, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing test coverage for the new replay schema field.
tests.rs only adds replay: None to two existing "fully-specified config" literals; there is no test case that parses a YAML replay: <path> value, exercises replay(), or verifies update() correctly merges a Some(path) value (all other fields have dedicated parsing/merge cases). This lines up with Codecov's report of low coverage on this file.
As per coding guidelines, "Add unit tests in fact/src/config/tests.rs for configuration schema changes in fact/src/config/mod.rs."
Also applies to: 82-114, 140-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fact/src/config/mod.rs` at line 44, The new replay field in the configuration
schema is missing dedicated test coverage. Update fact/src/config/tests.rs to
add cases that parse a YAML replay: <path> value, exercise the replay()
accessor, and verify update() merges a Some(path) value correctly; use the
existing Config and update() test patterns as the reference point. Also extend
the fully specified config fixtures that already include replay: None so they
cover the new field consistently, but make sure there is at least one explicit
test for parsing and merging replay.
Source: Coding guidelines
| #[derive(Debug, Clone, Default, Serialize, Deserialize)] | ||
| pub struct Process { | ||
| comm: String, | ||
| args: Vec<String>, | ||
| exe_path: PathBuf, | ||
| container_id: Option<String>, | ||
| uid: u32, | ||
| #[serde(skip_deserializing)] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Same skip-deserializing gap as Event.hostname — username is always blank on replay.
Same root cause as the Event.hostname field flagged in fact/src/event/mod.rs: this field will deserialize to "" for every replayed event with no reassignment happening. Worth applying the same fix (e.g., a default = "..." fn) if a live-username lookup is available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fact/src/event/process.rs` around lines 41 - 48, The Process.username field
is only marked skip_deserializing, so replayed events keep the default empty
value and never get repopulated. Update the Process struct’s serde handling to
mirror the Event.hostname fix by adding a default provider (or equivalent
reassignment path) for username in fact/src/event/process.rs, and ensure the
replay/deserialization flow restores a usable username instead of leaving it
blank.
| pub fn start( | ||
| task_set: &mut JoinSet<anyhow::Result<()>>, | ||
| path: &Path, | ||
| running: watch::Receiver<bool>, | ||
| ) -> anyhow::Result<mpsc::Receiver<Event>> { | ||
| anyhow::ensure!( | ||
| path.exists(), | ||
| "Replay file does not exist: {}", | ||
| path.display() | ||
| ); | ||
| let (tx, rx) = mpsc::channel(100); | ||
| let path = path.to_owned(); | ||
|
|
||
| task_set.spawn(async move { | ||
| let file = tokio::fs::File::open(&path) | ||
| .await | ||
| .with_context(|| format!("Failed to open replay file: {}", path.display()))?; | ||
| let reader = BufReader::new(file); | ||
| let mut lines = reader.lines(); | ||
|
|
||
| info!("Replaying events from {}", path.display()); | ||
| while let Some(line) = lines.next_line().await? { | ||
| if !*running.borrow() { | ||
| break; | ||
| } | ||
|
|
||
| let value: serde_json::Value = match serde_json::from_str(&line) { | ||
| Ok(v) => v, | ||
| Err(e) => { | ||
| warn!("Failed to parse JSON: {e}"); | ||
| continue; | ||
| } | ||
| }; | ||
| match serde_json::from_value::<Event>(value) { | ||
| Ok(event) => { | ||
| if tx.send(event).await.is_err() { | ||
| break; | ||
| } | ||
| } | ||
| Err(e) => warn!("Failed to deserialize event: {e}"), | ||
| } | ||
| } | ||
|
|
||
| info!("Replay finished"); | ||
| Ok(()) | ||
| }); | ||
|
|
||
| Ok(rx) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant files and surrounding context.
git ls-files fact/src/replay.rs fact/src/lib.rs
echo "--- replay.rs ---"
cat -n fact/src/replay.rs | sed -n '1,220p'
echo "--- lib.rs ---"
cat -n fact/src/lib.rs | sed -n '1,260p'Repository: stackrox/fact
Length of output: 8278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect downstream consumers to see how they handle shutdown and buffered drains.
echo "--- rate_limiter.rs ---"
wc -l fact/src/rate_limiter.rs
cat -n fact/src/rate_limiter.rs | sed -n '1,260p'
echo "--- output.rs ---"
wc -l fact/src/output.rs
cat -n fact/src/output.rs | sed -n '1,260p'Repository: stackrox/fact
Length of output: 4024
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate output module and inspect shutdown behavior around the downstream channel.
fd -a 'output.rs|mod.rs' fact/src
echo "--- possible output module candidates ---"
for f in $(git ls-files 'fact/src/**' | grep -E '(^|/)output(/|\.rs$)|(^|/)mod\.rs$' || true); do
echo "### $f"
wc -l "$f"
doneRepository: stackrox/fact
Length of output: 779
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the output module and inspect relevant logic.
git ls-files 'fact/src/**'
rg -n "pub struct Output|fn start|running.changed|recv\\(|send\\(" fact/src -g '!**/target/**'Repository: stackrox/fact
Length of output: 3394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the output pipeline implementation.
echo "--- src tree ---"
find fact/src -maxdepth 2 -type f | sort
echo "--- output-related files ---"
find fact/src -maxdepth 2 -type f | grep -E '/output(/|\.rs$)|/rate_limiter\.rs$|/replay\.rs$|/lib\.rs$' | sortRepository: stackrox/fact
Length of output: 860
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- fact/src/output/mod.rs ---"
cat -n fact/src/output/mod.rs | sed -n '1,220p'
echo "--- fact/src/output/stdout.rs ---"
cat -n fact/src/output/stdout.rs | sed -n '1,220p'Repository: stackrox/fact
Length of output: 5534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- fact/src/output/grpc.rs ---"
cat -n fact/src/output/grpc.rs | sed -n '1,340p'Repository: stackrox/fact
Length of output: 12895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the replay-specific test coverage and any shutdown expectations.
rg -n "replay|running.send\\(false\\)|join_next|EOF|drain|buffer" fact/src fact/tests -g '!**/target/**'Repository: stackrox/fact
Length of output: 2304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- fact/src/output/mod.rs ---"
cat -n fact/src/output/mod.rs | sed -n '1,220p'
echo "--- fact/src/output/stdout.rs ---"
cat -n fact/src/output/stdout.rs | sed -n '1,220p'
echo "--- fact/src/output/grpc.rs (relevant sections) ---"
cat -n fact/src/output/grpc.rs | sed -n '1,340p'Repository: stackrox/fact
Length of output: 18298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how replay mode is documented/positioned, to avoid overstating intent.
rg -n "replay|profil|profile" README* fact/src fact/src/config fact/src/main.rs -g '!**/target/**'Repository: stackrox/fact
Length of output: 1978
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the replay config documentation context.
cat -n fact/src/config/mod.rs | sed -n '716,734p'Repository: stackrox/fact
Length of output: 888
Replay EOF shouldn't stop the pipeline immediately. When this task finishes, run() treats it as shutdown and sends running = false, but RateLimiter/Output exit on running.changed() without draining their bounded queues. That can drop the tail of a replay, so keep replay completion separate from global shutdown or add an orderly drain step.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fact/src/replay.rs` around lines 13 - 61, The replay task in start() is being
treated as a global shutdown signal when it reaches EOF, which can stop
downstream consumers before their bounded queues are drained. Update the replay
completion path so finishing the file does not flip the shared running flag in
run(); instead, keep replay completion separate from shutdown or add an explicit
drain/flush step for RateLimiter and Output before exit. Use the start() task
and the run()/running.changed() shutdown flow to locate the fix.
2 participants
Description
Add a --replay CLI flag that reads pre-recorded events from a JSONL file instead of loading eBPF programs. This allows profiling the entire userspace pipeline with tools like valgrind DHAT that don't support BPF syscalls.
In replay mode, the BPF loader and HostScanner are bypassed entirely. Events flow directly from the JSONL reader through the RateLimiter and Output stages.
Changes:
Assisted-by: claude-opus-4.6 noreply@opencode.ai
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Run fact in stdout mode redirecting output to a file, then used that file to replay events on a mock server.
Summary by CodeRabbit